-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate o.e.equinox.common.tests to JUnit 4 #453
Migrate o.e.equinox.common.tests to JUnit 4 #453
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Heiko for this.
Looks good in general, just a few minor things that can be further improved.
public void testOperationCanceledExceptionAreHandled() { | ||
try { | ||
SafeRunner.run(() -> { | ||
throw new OperationCanceledException(); | ||
}); | ||
} catch (OperationCanceledException e) { | ||
fail("OperationCanceledException unexpectedly caught.", e); | ||
fail("OperationCanceledException unexpectedly caught."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just let the method throw the exception and let Junit handle it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, I don't like this fail
statement either, but I am not aware of a better, semantically equivalent realization in JUnit 4. For my main reason was semantics: Throwing the exception means that an occurrence of the exception will be treated as error. Calling fail
will let it be treated as a failure. While an error means that something unexpected (unrelated to the actual test) happened, a failure means that a property to be tested did not have the expected value/state. While in most cases exceptions indicate something unexpectedly going wrong (IOExceptions or the like), in this case the exception is subject to test. Consider that the test would not have any assert/fail statement without this, so there is no chance the test will ever fail (it can only error).
I actually miss some assertDoesNotThrow
statement like available in JUnit 5 to properly encode that in a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: that is the same reason why in other related PRs for JUnit migration I replaced some of the fail(String, Throwable)
calls with a wrapping of the exception into a thrown AssertionFailedError
(like it was done by the fail(String, Throwable)
method before). Like with the call to fail
here, this ensures that the tests fail with a desired message that also provides the causing exception instead of resulting in a test error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually miss some assertDoesNotThrow statement like available in JUnit 5 to properly encode that in a test.
Thanks for the explanation. I really appreciate your dedication for these details. In this case it semantically really makes sense to catch the exception. But if I would have designed the test-case I probably would have just let the exception be thrown.
While there is a semantic difference between errors and failures in tests in the end neither one should happen and from my experience the messages are often not that important to investigate the failure because in most cases one has to navigate to the code from the stack-trace any way.
Nevertheless I'm fine to keep this as it is and to replace it with that JUnit 5 method when migrating to JUnit 5.
That being said, I was told that we could also use JUnit-5, so if you don't know a reason against it and want to provide a follow up to migrate it to JUnit-5 it would be welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I really appreciate your dedication for these details.
And thank you for doing all the review work and merging the changes, Hannes! I know reviewing such refactoring/migration changes is not a very exciting task.
While there is a semantic difference between errors and failures in tests in the end neither one should happen and from my experience the messages are often not that important to investigate the failure because in most cases one has to navigate to the code from the stack-trace any way.
Yes, that totally conforms to my experience. I was also not that aware of that semantic difference for a long time and I think I do also not respect it properly in every case. And it will indeed not make a difference for investigating a failing test in most cases. But there are two other potential benefits in this distinction: first, it can help when designing tests to differentiate for onself whether some thrown exception is part of the test (i.e., having that exception thrown indicates a regression in the logic to be validated by the test) or whether that is something unrelated. Then, second, it can help to understand the purpose of a test when maintainig it, as it is made explicit what the test is supposed to validate (rather than potentially having a bunch of potentially thrown exceptions of which one relates to the functionality-under-test). But just take this as an input about my considerations. It's nothing I consider crucial for test design :-)
That being said, I was told that we could also use JUnit-5, so if you don't know a reason against it and want to provide a follow up to migrate it to JUnit-5 it would be welcome.
I would really like to finally have everything migrated to JUnit 5 to be up-to-date. I expect that to be a more "mechanical" task since all manual preparatory work (in particular getting rid of inheritance misuse) has already been done for JUnit 4 migration. I will probably do that once we have nothing depending in JUnit 3 anymore. The last thing to replace is the session test framework, which I will try to directly migrate to JUnit 5 since it heavily relies on test infrastructure (test suites, runners etc.) for which two migrations would probably lead to unnecessary, duplicate effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I really appreciate your dedication for these details.
And thank you for doing all the review work and merging the changes, Hannes! I know reviewing such refactoring/migration changes is not a very exciting task.
Indeed, but it is nice once one has completed it 😅
But just take this as an input about my considerations. It's nothing I consider crucial for test design :-)
Nevertheless interesting thoughts about the design of tests, which sound reasonable to me.
That being said, I was told that we could also use JUnit-5, so if you don't know a reason against it and want to provide a follow up to migrate it to JUnit-5 it would be welcome.
I would really like to finally have everything migrated to JUnit 5 to be up-to-date. I expect that to be a more "mechanical" task since all manual preparatory work (in particular getting rid of inheritance misuse) has already been done for JUnit 4 migration. I will probably do that once we have nothing depending in JUnit 3 anymore. The last thing to replace is the session test framework, which I will try to directly migrate to JUnit 5 since it heavily relies on test infrastructure (test suites, runners etc.) for which two migrations would probably lead to unnecessary, duplicate effort.
Sounds good.
I suggest to start with a small test project like this one for o.e.equinox.common and have this merged and at least one I-build run with it in order to verify JUnit-5 really works in the entire chain.
Somebody told me that there is already a test-project in the SDK that uses JUnit-5, but I have not yet looked that up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to start with a small test project like this one for o.e.equinox.common and have this merged and at least one I-build run with it in order to verify JUnit-5 really works in the entire chain.
Yes, that makes sense. I once re-enabled org.eclipse.ui.tests.rcp
and tried to directly migrate to JUnit 5 but had some issues with Tycho that I did not resolve back then. Probably I will start with that project as it's small and I still have the migrated code.
Somebody told me that there is already a test-project in the SDK that uses JUnit-5, but I have not yet looked that up.
Thanks for the hint 👍 org.eclipse.e4.ui.tests.css.core
already uses JUnit 5 successfully.
...org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/ServiceCallerTest.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.equinox.common.tests/src/org/eclipse/equinox/common/tests/URIUtilTest.java
Outdated
Show resolved
Hide resolved
6ca0bfe
to
53398de
Compare
53398de
to
69e4ff4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good and the number of tests remained the same.
Thank you!
@HeikoKlare if you see any build issues please let me know so we can analyze / fix that. |
Yes, if I find something, I'll let you know. But I guess the mentioned case was rather an error by user 😉 |
@laeubi FYI: I just recapped what prevented me from a migration to JUnit 5 yet: Tycho cannot execute suites (or at least I did not find what configuration is missing for that yet). The issue is also documented in eclipse-tycho/tycho#2462. I will try to provide a reproducing integration test for Tycho. |
@Test
,@Before ,
@After`TestCase
andCoreTest
assertThrows()
This makes
org.eclipse.equinox.common.tests
completely independent from JUnit 3 and particularly removes the dependency toCoreTest
in that bundle (see eclipse-platform/eclipse.platform.releng.aggregator#1684)